add future support - #38
andrewgazelka wants to merge 6 commits into
Conversation
|
Also—can someone verify the MSRV? I believe it should be |
55be8f5 to
015c293
Compare
|
forgot to update the GitHub workflow. Fixed. |
|
removed pin_project because it required using a higher MSRV |
eldruin
left a comment
eldruin
left a comment
There was a problem hiding this comment.
This is interesting, thank you!
In the grand scheme of things, we have thought of deprecating this crate once everything that embedded-hal-async needs is stable and we can merge it into embedded-hal, featuring traits which actually return futures and so on.
However, I would be open to adding Future support to nb if it provides some value, at least in the mean time, which can be long, or eases the migration.
Could you add documentation detailing how this PR would work for a driver and MCU HAL implementation, similar to what is already in place?
bc7d080 to
83ecc05
Compare
| impl<Ok, Err, Gen: FnMut() -> Result<Ok, Err>> Future for NbFuture<Ok, Err, Gen> { | ||
| type Output = core::result::Result<Ok, Err>; | ||
|
|
||
| fn poll(self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<Self::Output> { |
There was a problem hiding this comment.
hey thanks for the PR! given that the waker isn't getting propagated here i wouldn't expect the scheduler to poll this again, and always hitting wake runs into performance problems on non-embedded platforms... does this behave as expected / how're you using this at the moment?
(cc. @Dirbaio the futures-whisper)
There was a problem hiding this comment.
Yes, it'll hang with most executors out there due to not waking the waker. Docs on Future::poll() say supporting wakers is mandatory.
It'll only work with very dumb executors that do loop { fut.poll(cx) } ignoring the waker, spinning the CPU at 100%.
There was a problem hiding this comment.
There's one way around it, which is immediately waking ourselves on poll: cx.waker().wake_by_ref(). It'll still spin the CPU at 100% but at least it won't hang. In theory this might cause other tasks to get starved of CPU, but in practice executors usually handle a "self-wake" like this by moving the task to the back of the queue, so other tasks still get to run. I don't think it's guaranteed though.
IMHO the way forward is HALs implementing the async traits, and deprecate nb. This allows HALs to wake the wakers from irqs, avoiding spinning the cpu at 100%. It also allows using DMA (you can't use DMA soundly with nb).
There's another issue: the way nb is currently used, the task is polled for each byte. This makes it too easy to lose data if the task isn't polled fast enough, as the hardware buffers are usually tiny. If HALs impl the futures instead, they have powerful tools to avoid that (irqs, dma).
|
I'm closing this PR as it's very outdated and won't be merged. |
6 participants
No description provided.